Skip to content

fix(tool/code_search): guard option-like refs in buildGrepArgs - #727

Merged
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
aalhadxx:fix/code-search-end-of-options
Aug 5, 2026
Merged

fix(tool/code_search): guard option-like refs in buildGrepArgs#727
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
aalhadxx:fix/code-search-end-of-options

Conversation

@aalhadxx

@aalhadxx aalhadxx commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #645

Description

git grep on Git < 2.45 does not accept --end-of-options between the pattern and revision arguments. It treats --end-of-options as a revision and fails with:

fatal: ambiguous argument '--end-of-options': unknown revision or path not in the working tree.

This PR removes --end-of-options from buildGrepArgs for git grep and moves the defense-in-depth check (reject refs starting with -) into buildGrepArgs() so the guard is co-located with argument construction.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

How Has This Been Tested?

  • make test passes locally
  • Manual testing (described below)

I ran the internal/tool tests locally with go test ./internal/tool/ -run 'CodeSearch|BuildGrepArgs|GitGrep' and all 25 tests passed. The new test TestBuildGrepArgs_RejectsOptionLikeRef specifically covers the nil-return path when a ref starts with -.

Describe the tests you ran to verify your changes:

  • TestBuildGrepArgs_RejectsOptionLikeRef — verifies buildGrepArgs returns nil for option-like refs
  • TestGitGrep_RejectsOptionLikeRef — verifies gitGrep returns the correct error string
  • TestGitGrep_OptionLikeRefDoesNotLaunchPager — verifies no file is created when an option-like ref is passed
  • All existing BuildGrepArgs and GitGrep tests continue to pass

Include relevant details about your test configuration:

  • Go 1.23.4 windows/amd64
  • Git 2.55.0.windows.3

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have signed the CLA

Related Issues

Fixes #645
Addresses review feedback from @lizhengfeng101 in #663

Security Note

Upstream validateReviewRefs (review_cmd.go:340-365) already rejects refs starting with - and verifies the ref resolves to a real commit via git rev-parse --verify. The check inside buildGrepArgs is defense-in-depth in case that function is ever called from a new code path. A comment explains why --end-of-options can't be used here: git grep < 2.45 does not support --end-of-options before the revision.

Moves the defense-in-depth check from gitGrep() into buildGrepArgs()
so the guard is co-located with argument construction.

Adds a comment explaining why --end-of-options can't be used here:
git grep < 2.45 does not support --end-of-options before the revision.

Adds TestBuildGrepArgs_RejectsOptionLikeRef.

Signed-off-by: Aalhad <aalhadxx@users.noreply.github.com>
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

Comment thread internal/tool/code_search.go Outdated
@@ -1,4 +1,4 @@
package tool
package tool

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
BOM (Byte Order Mark) character added: The diff shows a BOM (U+FEFF) has been added before package tool. This can cause compilation issues with older Go versions or tools that don't expect a BOM. Go source files should not have a BOM as Go uses UTF-8 natively. This appears to be an accidental change.

@aalhadxx
aalhadxx force-pushed the fix/code-search-end-of-options branch from 7736264 to deaf560 Compare August 5, 2026 04:31

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 85e8639 into alibaba:main Aug 5, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

code_search fails in range/commit reviews because git grep treats --end-of-options as a revision

2 participants